feat: Add timeout to container task #3314
Conversation
Narwhal-fish
requested review from
cosmicBboy,
davidmirror-ops,
eapolinario,
kumare3,
pingsutw,
samhita-alla and
wild-endeavor
as code owners
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
Narwhal-fish
force-pushed
the
add-timeout-to-containerTask
branch
2 times, most recently
from
2c19642 to
dccb4ee
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3314 +/- ##
==========================================
- Coverage 85.26% 83.58% -1.68%
==========================================
Files 386 3 -383
Lines 30276 195 -30081
Branches 2969 0 -2969
==========================================
- Hits 25814 163 -25651
+ Misses 3615 32 -3583
+ Partials 847 0 -847 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'm not sure if this is the correct approach. For regular Python tasks the timeout is passed through the task metadata and queueing time is counted towards the overall task timeout. It looks like this implementation only sets the active deadline seconds on the pod which will only affect the underlying container when it eventually runs. So I am a bit worried about the difference in semantics. Is it possible to instead or also plumb the timeout through the task metadata? |
Sorry for the late reply. I'll modify the implementation to plumb the timeout through the task metadata so that it includes queueing time and follows the same execution semantics as regular Python tasks. |
…n duration Signed-off-by: catfish <[email protected]>
Signed-off-by: catfish <[email protected]>
…delta Signed-off-by: catfish <[email protected]>
Signed-off-by: catfish <[email protected]>
Signed-off-by: catfish <[email protected]>
…emantics Ensure ContainerTask timeout behavior matches regular Python tasks by using TaskMetadata mechanism. Signed-off-by: catfish <[email protected]>
Narwhal-fish
force-pushed
the
add-timeout-to-containerTask
branch
from
38cdee3 to
0aa2567
Compare
|
Bito Automatic Review Failed - Technical Failure |
3 participants
Tracking issue
Related to flyteorg/flyte#6351
Why are the changes needed?
Currently,
ContainerTasklacks built-in timeout functionality, which is available for regular Python tasks via the@taskdecorator.What changes were proposed in this pull request?
This PR adds a
timeoutparameter toContainerTaskthat acceptsdatetime.timedeltaobjects:How was this patch tested?
test_container_task_timeout(): Tests local execution timeout with timedeltatest_container_task_timeout_k8s_serialization(): Verifies K8sactiveDeadlineSecondsserializationtest_container_task_no_timeout(): Ensures normal execution works with sufficient timeoutSetup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This pull request enhances the ContainerTask functionality by adding a timeout parameter for maximum execution duration, enabling it to accept a timeout parameter via a datetime.timedelta object. Modifications include updates to the ContainerTask class and Kubernetes pod specification, along with new unit tests to verify the feature's functionality during local execution and Kubernetes serialization, enhancing the task's capabilities to match those of standard Python tasks.